Skip to content

Conversation

@ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Dec 13, 2024

Hi,

can you please review patch to have import to return nil. It addresses #1174.

I haven't included a test for this case, nor mentioned it in the docstring, implying the return value is unspecified. I'm not sure this is something we want to fix at the API level (although I did request it as an aesthetic addition).

@ikappaki ikappaki marked this pull request as draft December 13, 2024 21:24
@ikappaki ikappaki marked this pull request as ready for review December 13, 2024 21:33
@ikappaki
Copy link
Contributor Author

I have no idea why the CI, including generating the docs failed, it works fine locally even after I recreated the venv. Perhaps a restart will fix it?

@ikappaki
Copy link
Contributor Author

I have no idea why the CI, including generating the docs failed, it works fine locally even after I recreated the venv. Perhaps a restart will fix it?

I've managed to reproduce the issue, it needs wiping out all the files and environment outside of the project (I used git -xfd), as like starting with a fresh checkout.

It is difficult to dtermine the cause just by looking at the trace and the code it points to. The failure occurs when attempting to require basilisp.io from core.lpy at start up

;; This is a circular import since namespaces using 'ns will always try to refer all
;; symbols from 'basilisp.core. This should be fine since we won't be using anything
;; from core defined below this section, but still this is a bad pattern and should
;; not be replicated.
(require 'basilisp.io)

the error is NameError: name 'eval_' is not defined. Did you mean: 'eval'?. A insights on what this might mean?

$ poetry run basilisp repl
Traceback (most recent call last):
  File "D:\src\basilisp\src\basilisp\importer.py", line 408, in exec_module
    self._exec_cached_module(fullname, spec.loader_state, path_stats, ns)
  File "D:\src\basilisp\src\basilisp\importer.py", line 312, in _exec_cached_module
    cache_data = self.get_data(cache_filename)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\src\basilisp\src\basilisp\importer.py", line 216, in get_data
    with open(path, mode="r+b") as f:
         ^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'D:\\src\\basilisp\\src\\basilisp\\__pycache__\\core.cpython-311.lpyc'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "D:\src\basilisp\src\basilisp\importer.py", line 408, in exec_module
    self._exec_cached_module(fullname, spec.loader_state, path_stats, ns)
  File "D:\src\basilisp\src\basilisp\importer.py", line 312, in _exec_cached_module
    cache_data = self.get_data(cache_filename)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\src\basilisp\src\basilisp\importer.py", line 216, in get_data
    with open(path, mode="r+b") as f:
         ^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'D:\\src\\basilisp\\src\\basilisp\\__pycache__\\io.cpython-311.lpyc'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "D:\src\basilisp\src\basilisp\cli.py", line 833, in invoke_cli
    parsed_args.handler(parser, parsed_args)
  File "D:\src\basilisp\src\basilisp\cli.py", line 533, in repl
    basilisp.init(opts)
  File "D:\src\basilisp\src\basilisp\main.py", line 29, in init
    importlib.import_module("basilisp.core")
  File "C:\local\Python311\Lib\importlib\__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1204, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1176, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1147, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
  File "D:\src\basilisp\src\basilisp\importer.py", line 411, in exec_module
    self._exec_module(fullname, spec.loader_state, path_stats, ns)
  File "D:\src\basilisp\src\basilisp\importer.py", line 354, in _exec_module
    compiler.compile_module(
  File "D:\src\basilisp\src\basilisp\lang\compiler\__init__.py", line 264, in compile_module
    _incremental_compile_module(
  File "D:\src\basilisp\src\basilisp\lang\compiler\__init__.py", line 225, in _incremental_compile_module
    exec(bytecode, ns.module.__dict__)  # pylint: disable=exec-used  # nosec 6102
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\src\basilisp\src\basilisp\core.lpy", line 7219, in <module>
    (require 'basilisp.io)
  File "D:\src\basilisp\src\basilisp\core.lpy", line 5139, in require
    (doseq [libspec (map require-libspec libspecs)]
  File "D:\src\basilisp\src\basilisp\core.lpy", line 2524, in dorun
    (defn dorun
  File "D:\src\basilisp\src\basilisp\core.lpy", line 2533, in dorun__arity1
    (when (seq ptr)
          ^^^^^^^^^
  File "C:\local\Python311\Lib\functools.py", line 909, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\src\basilisp\src\basilisp\lang\seq.py", line 270, in _to_seq_lazyseq
    return o.seq()
           ^^^^^^^
  File "D:\src\basilisp\src\basilisp\lang\seq.py", line 173, in seq
    self._compute_seq()
  File "D:\src\basilisp\src\basilisp\lang\seq.py", line 168, in _compute_seq
    self._obj = gen()
                ^^^^^
  File "D:\src\basilisp\src\basilisp\core.lpy", line 5145, in ____require__for_4140_4156__lisp_fn_4158
    (require-lib current-ns libspec flags)
  File "D:\src\basilisp\src\basilisp\core.lpy", line 5033, in require_lib
    (eval (list 'require*
  File "D:\src\basilisp\src\basilisp\core.lpy", line 4562, in eval_
    (defn eval
  File "D:\src\basilisp\src\basilisp\core.lpy", line 4571, in eval___arity2
    (basilisp.lang.compiler/compile-and-exec-form form
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\src\basilisp\src\basilisp\lang\compiler\__init__.py", line 189, in compile_and_exec_form
    exec(bytecode, ns.module.__dict__)  # pylint: disable=exec-used  # nosec 6102
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<Eval Input>", line 1, in <module>
  File "D:\src\basilisp\src\basilisp\lang\runtime.py", line 709, in require
    ns_module = importlib.import_module(munge(ns_name))
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\local\Python311\Lib\importlib\__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1204, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1176, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1147, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
  File "D:\src\basilisp\src\basilisp\importer.py", line 411, in exec_module
    self._exec_module(fullname, spec.loader_state, path_stats, ns)
  File "D:\src\basilisp\src\basilisp\importer.py", line 354, in _exec_module
    compiler.compile_module(
  File "D:\src\basilisp\src\basilisp\lang\compiler\__init__.py", line 264, in compile_module
    _incremental_compile_module(
  File "D:\src\basilisp\src\basilisp\lang\compiler\__init__.py", line 225, in _incremental_compile_module
    exec(bytecode, ns.module.__dict__)  # pylint: disable=exec-used  # nosec 6102
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\src\basilisp\src\basilisp\io.lpy", line 1, in <module>
    (ns basilisp.io
NameError: name 'eval_' is not defined. Did you mean: 'eval'?

Thanks

@ikappaki
Copy link
Contributor Author

ikappaki commented Dec 16, 2024

I've done a little digging with the generated code based on the succesful example you've provided in the review comments, and the issue appears to be with the eval being inside a do form or not at all:

This is the reference Basilisp code from core.lpy whose generated python is listed below:

;;;;;;;;;;;;;;;;;;
;; IO Utilities ;;
;;;;;;;;;;;;;;;;;;

;; This is a circular import since namespaces using 'ns will always try to refer all
;; symbols from 'basilisp.core. This should be fine since we won't be using anything
;; from core defined below this section, but still this is a bad pattern and should
;; not be replicated.
(require 'basilisp.io)

Here is the generated python when it succeeds (eval is at top level). Notice the Var_31.find expression at the beginning, there is no eval_ calls.

(let [form `(do (import* ~@modules)
                  nil)]
    `(eval '~form  *ns*))
#...
Var_31.find_safe(sym_27.symbol('in-ns', ns='basilisp.core')).value(sym_27.symbol('basilisp.io', meta=None))
Var_31.find_safe(sym_27.symbol('swap!', ns='basilisp.core')).value(Var_31.find_safe(sym_27.symbol('*loaded-libs*', ns='basilisp.core')).value, Var_31.find_safe(sym_27.symbol('conj', ns='basilisp.core')).value, sym_27.symbol('basilisp.io', meta=None))
Var_31.find_safe(sym_27.symbol('the-ns', ns='basilisp.core')).value(sym_27.symbol('basilisp.io', meta=None)).reset_meta(lmap_18.map({kw_16.keyword_from_hash(2062578673702846720, 'doc'): 'Polymorphic IO functions.\n\n  Functions in this namespace can provide readers and writers for both text and binary\n  streams from a wide variety of different input types as well as utility functions for\n  interacting with the filesystem.'}))
Var_31.find_safe(sym_27.symbol('refer-basilisp', ns='basilisp.core')).value()
Var_31.find_safe(sym_27.symbol('require', ns='basilisp.core')).value(vec_29.vector([sym_27.symbol('basilisp.string', meta=None), kw_16.keyword_from_hash(-9034785645437277742, 'as'), sym_27.symbol('str', meta=None)], meta=None))
Var_31.find_safe(sym_27.symbol('eval', ns='basilisp.core')).value(llist_17.list([sym_27.symbol('do'), llist_17.list([sym_27.symbol('import*'), sym_27.symbol('os.path', meta=None)]), None]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)
Var_31.find_safe(sym_27.symbol('eval', ns='basilisp.core')).value(llist_17.list([sym_27.symbol('do'), llist_17.list([sym_27.symbol('import*'), sym_27.symbol('pathlib', meta=None)]), None]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)
Var_31.find_safe(sym_27.symbol('eval', ns='basilisp.core')).value(llist_17.list([sym_27.symbol('do'), llist_17.list([sym_27.symbol('import*'), sym_27.symbol('shutil', meta=None)]), None]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)
Var_31.find_safe(sym_27.symbol('eval', ns='basilisp.core')).value(llist_17.list([sym_27.symbol('do'), llist_17.list([sym_27.symbol('import*'), sym_27.symbol('urllib.parse', meta=None)]), None]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)
Var_31.find_safe(sym_27.symbol('eval', ns='basilisp.core')).value(llist_17.list([sym_27.symbol('do'), llist_17.list([sym_27.symbol('import*'), sym_27.symbol('urllib.request', meta=None)]), None]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)

requiring_ns_5483 = _NS.value
try:
    basilisp_core = requiring_ns_5483.require('basilisp.core')
finally:
    _NS.set_value(requiring_ns_5483)
del requiring_ns_5483

def __lisp_expr___5482():
    return basilisp_core

requiring_ns_5485 = _NS.value
try:
    basilisp_string = requiring_ns_5485.require('basilisp.string', sym_27.symbol('str'))
finally:
    _NS.set_value(requiring_ns_5485)
del requiring_ns_5485

def __lisp_expr___5484():
    return basilisp_string

import basilisp
import attr as attr_3
import basilisp.lang.atom as atom_9
import basilisp.lang.compiler as compiler_10
import basilisp.lang.delay as delay_12
import basilisp.lang.exception as exc_13
import basilisp.lang.futures as futures_14
import basilisp.lang.interfaces as interfaces_15
import basilisp.lang.keyword as kw_16
import basilisp.lang.list as llist_17
import basilisp.lang.map as lmap_18
import basilisp.lang.multifn as multifn_19
import basilisp.lang.promise as promise_20
import basilisp.lang.queue as queue_21
import basilisp.lang.reader as reader_22
import basilisp.lang.reduced as reduced_23
import basilisp.lang.runtime as runtime_24
import basilisp.lang.seq as seq_25
import basilisp.lang.set as lset_26
import basilisp.lang.symbol as sym_27
import basilisp.lang.tagged as tagged_28
import basilisp.lang.util as langutil_33
import basilisp.lang.vector as vec_29
import basilisp.lang.volatile as volatile_30
import builtins as builtins_4
import functools as functools_5
import importlib as importlib_6
import io as io_7
import operator as operator_2
import sys as sys_8
from basilisp.lang.runtime import Var as Var_31
from typing import Union as Union_32
_NS = Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core'))

requiring_ns_5487 = _NS.value
try:
    basilisp_core = requiring_ns_5487.require('basilisp.core')
finally:
    _NS.set_value(requiring_ns_5487)
del requiring_ns_5487

def __lisp_expr___5486():
    return basilisp_core

re_EuzL3Z = builtins_4.__import__('re')
_NS.value.add_import(sym_27.symbol('re'), re_EuzL3Z)

def __lisp_expr___5488():
    return re_EuzL3Z

typing_al35Kw = builtins_4.__import__('typing')
_NS.value.add_import(sym_27.symbol('typing'), typing_al35Kw)

def __lisp_expr___5489():
    return typing_al35Kw

# ... after about a thousand lines of code more, eval_ is defined

@runtime_24._basilisp_fn(arities=(1, 2))
def eval_(*multi_arity_args):
    nargs_3726 = len(multi_arity_args)
    arity_3727 = eval__dispatch_map.get(nargs_3726)
    if None is not arity_3727:
        return arity_3727(*multi_arity_args)
    raise basilisp.lang.runtime.RuntimeException('Wrong number of args passed to function: eval_', nargs_3726)

# and goes on ...

and here is the generated python code of using (do (eval ....

(let [form `(import* ~@modules)]
    `(do (eval '~form  *ns*)
         nil))

Notice the corresponding Var_31.find_safe calls at the beginning, the imports are now call eval_ (i.e. (defn eval). So it appears that the do form forces the imported namespaces to use eval_, even though it has not been defined yet?

Var_31.find_safe(sym_27.symbol('in-ns', ns='basilisp.core')).value(sym_27.symbol('basilisp.io', meta=None))
Var_31.find_safe(sym_27.symbol('swap!', ns='basilisp.core')).value(Var_31.find_safe(sym_27.symbol('*loaded-libs*', ns='basilisp.core')).value, Var_31.find_safe(sym_27.symbol('conj', ns='basilisp.core')).value, sym_27.symbol('basilisp.io', meta=None))
Var_31.find_safe(sym_27.symbol('the-ns', ns='basilisp.core')).value(sym_27.symbol('basilisp.io', meta=None)).reset_meta(lmap_18.map({kw_16.keyword_from_hash(5225872901053642968, 'doc'): 'Polymorphic IO functions.\n\n  Functions in this namespace can provide readers and writers for both text and binary\n  streams from a wide variety of different input types as well as utility functions for\n  interacting with the filesystem.'}))
Var_31.find_safe(sym_27.symbol('refer-basilisp', ns='basilisp.core')).value()
Var_31.find_safe(sym_27.symbol('require', ns='basilisp.core')).value(vec_29.vector([sym_27.symbol('basilisp.string', meta=None), kw_16.keyword_from_hash(117104300546238346, 'as'), sym_27.symbol('str', meta=None)], meta=None))
eval_(llist_17.list([sym_27.symbol('import*'), sym_27.symbol('os.path', meta=None)]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)
eval_(llist_17.list([sym_27.symbol('import*'), sym_27.symbol('pathlib', meta=None)]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)
eval_(llist_17.list([sym_27.symbol('import*'), sym_27.symbol('shutil', meta=None)]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)
eval_(llist_17.list([sym_27.symbol('import*'), sym_27.symbol('urllib.parse', meta=None)]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)
eval_(llist_17.list([sym_27.symbol('import*'), sym_27.symbol('urllib.request', meta=None)]), Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core')).value)

requiring_ns_5483 = _NS.value
try:
    basilisp_core = requiring_ns_5483.require('basilisp.core')
finally:
    _NS.set_value(requiring_ns_5483)
del requiring_ns_5483

def __lisp_expr___5482():
    return basilisp_core

requiring_ns_5485 = _NS.value
try:
    basilisp_string = requiring_ns_5485.require('basilisp.string', sym_27.symbol('str'))
finally:
    _NS.set_value(requiring_ns_5485)
del requiring_ns_5485

def __lisp_expr___5484():
    return basilisp_string

import basilisp
import attr as attr_3
import basilisp.lang.atom as atom_9
import basilisp.lang.compiler as compiler_10
import basilisp.lang.delay as delay_12
import basilisp.lang.exception as exc_13
import basilisp.lang.futures as futures_14
import basilisp.lang.interfaces as interfaces_15
import basilisp.lang.keyword as kw_16
import basilisp.lang.list as llist_17
import basilisp.lang.map as lmap_18
import basilisp.lang.multifn as multifn_19
import basilisp.lang.promise as promise_20
import basilisp.lang.queue as queue_21
import basilisp.lang.reader as reader_22
import basilisp.lang.reduced as reduced_23
import basilisp.lang.runtime as runtime_24
import basilisp.lang.seq as seq_25
import basilisp.lang.set as lset_26
import basilisp.lang.symbol as sym_27
import basilisp.lang.tagged as tagged_28
import basilisp.lang.util as langutil_33
import basilisp.lang.vector as vec_29
import basilisp.lang.volatile as volatile_30
import builtins as builtins_4
import functools as functools_5
import importlib as importlib_6
import io as io_7
import operator as operator_2
import sys as sys_8
from basilisp.lang.runtime import Var as Var_31
from typing import Union as Union_32
_NS = Var_31.find_safe(sym_27.symbol('*ns*', ns='basilisp.core'))

requiring_ns_5487 = _NS.value
try:
    basilisp_core = requiring_ns_5487.require('basilisp.core')
finally:
    _NS.set_value(requiring_ns_5487)
del requiring_ns_5487

def __lisp_expr___5486():
    return basilisp_core

re_EuzL3Z = builtins_4.__import__('re')
_NS.value.add_import(sym_27.symbol('re'), re_EuzL3Z)

def __lisp_expr___5488():
    return re_EuzL3Z

typing_al35Kw = builtins_4.__import__('typing')
_NS.value.add_import(sym_27.symbol('typing'), typing_al35Kw)

def __lisp_expr___5489():
    return typing_al35Kw
# end of generated code because of error

With the error being:

#...
  File "D:\src\basilisp\src\basilisp\io.lpy", line 1, in <module>
    (ns basilisp.io
NameError: name 'eval_' is not defined. Did you mean: 'eval'?

@ikappaki
Copy link
Contributor Author

I believe we are hitting a special do handling case in the function below, which enables var indirection for do even though basilisp.core is still being loading. However, since the code for the eval function which the form is referencing is not yet generated, this lead to a failure. This interpretation appears to me to align with the comment, which to me it suggests the same.

This also align with the behavior we see when moving the do inside the eval statement, as you suggested.

Assuming this is the case, I'm unsure how this can be resolved. Should do blocks be prohibited being used in the core namespace before the function code they refer to is fully generated?

That said, please take this analysis with a grain of salt. I'm attempting to connect the dots but I lack the full picture.

def _wrap_override_var_indirection(
    f: "PyASTGenerator[T_node, P_generator, T_pynode]",
) -> "PyASTGenerator[T_node, P_generator, T_pynode]":
    """
    Wrap a Node generator to apply a special override requiring Var indirection
    for any Var accesses generated within `do` blocks which are marked with the
    ^:use-var-indirection metadata.

    This is needed to account for the `ns` macro, which is the first form in most
    standard Namespaces. When Basilisp `require`s a Namespace, it (like in Clojure)
    simply loads the file and lets that Namespace's `ns` macro create the new
    Namespace and perform any setup. However, the Basilisp compiler desperately
    tries to emit "smarter" Python code which avoids using `Var.find` whenever
    the resolved symbol can be safely called directly from the generated Python
    module. Without this hack, the compiler will emit code during macroexpansion
    to access `basilisp.core` functions used in the `ns` macro directly, even
    though they will not be available yet in the target Namespace module.
    """

    @wraps(f)
    def _wrapped_do(
        ctx: GeneratorContext,
        node: T_node,
        *args: P_generator.args,
        **kwargs: P_generator.kwargs,
    ) -> GeneratedPyAST[T_pynode]:
        if isinstance(node, Do) and node.use_var_indirection:
            with ctx.with_var_indirection_override():
                return f(ctx, cast(T_node, node), *args, **kwargs)
        else:
            with ctx.with_var_indirection_override(False):
                return f(ctx, node, *args, **kwargs)

    return _wrapped_do

@ikappaki
Copy link
Contributor Author

I've relocated the do inside the eval form as per your suggestion, given the above (yet unverified) analysis I provided above seems reasonable to me.

@chrisrink10
Copy link
Member

I've relocated the do inside the eval form as per your suggestion, given the above (yet unverified) analysis I provided above seems reasonable to me.

Thank you for the analysis. What you're saying seems plausible at least, though it probably shouldn't happen. I can try to file a separate ticket to investigate it outside of this PR.

@chrisrink10 chrisrink10 merged commit 841392d into basilisp-lang:main Dec 18, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants